Audit of warn! and error! by JasonAtClockwork · Pull Request #5640 · clockworklabs/SpacetimeDB

JasonAtClockwork · 2026-07-31T20:37:50Z

Description of Changes

Reviewed all warn! and error! lines for host side issues to get ready for alerting.

API and ABI breaking changes

No breaking changes.

Expected complexity level and risk

1 - Only changes to logging

Testing

Ran cargo test against impacted crates

JasonAtClockwork · 2026-07-31T22:58:50Z

        .map_err(|e| {
+            // TODO: Review log level after user SQL errors can be distinguished from internal database failures.
            log::warn!("{e}");
            (StatusCode::BAD_REQUEST, e.to_string())


Same as other areas we might want to split this up so we can use error! if there is a database failure.

Can you create a follow-up ticket?

Follow-up created: #5690

I did go down the rabbit-hole of all the possible issues and decided to make the follow-up simple instead of declaring all possible areas of concern.

JasonAtClockwork · 2026-07-31T22:59:39Z

+// TODO: Review each caller and split this helper by the appropriate log severity.
 pub fn log_and_500(e: impl std::fmt::Display) -> ErrorResponse {
    log::error!("internal error: {e:#}");
    (StatusCode::INTERNAL_SERVER_ERROR, format!("{e:#}")).into()


This seems to be very broadly used which makes it hard to know if it should be reduced from error! my gut reaction is to change it to warn! and slowly clean up it's use?

Please change to warn for now, and any place you're already editing that has a log_and_500 directly next to its own more specific log line, remove the log_and_500 call and replace with directly returning the error response. You may want to move the error response into a new helper function.

JasonAtClockwork · 2026-07-31T23:14:31Z

+            // TODO: Review log level after configured issuers can be distinguished from arbitrary token issuers.
            log::warn!("Error fetching public key for issuer {raw_issuer}: {e:?}");


This one I'm not 100% sure if we need to split up or count everything as warn!

I have no clue. At this stage I'm unconcerned.

JasonAtClockwork · 2026-07-31T23:15:12Z

+                // TODO: Review log level after migration/user errors can be distinguished from internal database failures.
                log::warn!("Database update failed: {} @ {}", e, stdb.database_identity());
                system_logger.warn(&format!("Database update failed: {e}"));
                let (_, tx_metrics, reducer) = stdb.rollback_mut_tx(tx);


Another spot where it looks like real failures can be mixed in with user issues.

Make a follow-up?

Added a follow-up: #5691

JasonAtClockwork · 2026-07-31T23:15:37Z

+                        // TODO: Review log level after guest view errors can be distinguished from materialization failures.
                        log::error!("Error materializing view `{view_name}`: {err:?}");
                        ViewOutcome::Failed(format!("Error materializing view `{view_name}`: {err}"))


This one looked similar as above but I'm not certain if it's true that it can be mixed here.

Another follow-up ticket? And, can @joshua-spacetime weigh in as to whether user errors can happen here, which I assume would be if a view function panics.

Yes if a view function panics. Primary key violations too.

Ok, then we should drop this to warn.

Dropped to warn and added a follow-up: #5692

bfops · 2026-08-03T08:02:38Z

FYI #5320 is about to merge which tweaks some logs as well

Signed-off-by: Jason Larabie <jason@clockworklabs.io>

gefjon · 2026-08-04T16:29:33Z

        .map_err(|e| {
+            // TODO: Review log level after user SQL errors can be distinguished from internal database failures.
            log::warn!("{e}");
            (StatusCode::BAD_REQUEST, e.to_string())


Can you create a follow-up ticket?

gefjon · 2026-08-04T16:31:06Z

+// TODO: Review each caller and split this helper by the appropriate log severity.
 pub fn log_and_500(e: impl std::fmt::Display) -> ErrorResponse {
    log::error!("internal error: {e:#}");
    (StatusCode::INTERNAL_SERVER_ERROR, format!("{e:#}")).into()


Please change to warn for now, and any place you're already editing that has a log_and_500 directly next to its own more specific log line, remove the log_and_500 call and replace with directly returning the error response. You may want to move the error response into a new helper function.

gefjon · 2026-08-04T16:31:57Z

+            // TODO: Review log level after configured issuers can be distinguished from arbitrary token issuers.
            log::warn!("Error fetching public key for issuer {raw_issuer}: {e:?}");


I have no clue. At this stage I'm unconcerned.

gefjon · 2026-08-04T16:42:53Z

            Some(ptr) => self.delete(ST_CLIENT_ID, ptr).map(drop)?,
            _ => {
-                log::error!(
+                log::warn!(


@joshua-spacetime how can this (and the other previously error! logs in this file) happen, and how bad is it?

Not bad. They only happen if the row is missing, which just means it was cleaned up previously. They don't need to be error logs.

How about we drop to info, then?

Switched this to info

gefjon · 2026-08-04T16:43:29Z

            }
            Err(e) => {
+                // TODO: Review log level after retryable cleanup errors can be distinguished from internal database failures.
                log::error!(


What user errors can get mixed in here?

I've removed the comment.

This was poorly written and I'm not sure it's that useful after a re-review. The clear_expired_views can error if lookup_st_view() fails which can mean the view is already cleared. That would be the only case I can see here that it could be warn or lower.

gefjon · 2026-08-04T16:44:03Z

                    let res = self
                        .maybe_take_snapshot(database_state.as_ref())
                        .await
+                        // TODO: Review log level after lifecycle errors can be distinguished from snapshot creation failures.


Please create a follow-up ticket.

Added follow-up: #5693

gefjon · 2026-08-04T16:45:01Z

+            // TODO: Review log level after client SQL errors can be distinguished from internal database failures.
            log::error!("PG: Error for database {database}: {err}");


Downgrade to warn, at least until the TODO is done. Please make a follow-up ticket, but mark it P3.

Changed to warn and added follow-up: #5695

gefjon · 2026-08-04T16:45:30Z

+                        // TODO: Do not log the supplied password/token; then classify credential errors separately from provider failures.
                        log::error!(


Please downgrade to warn, and make a P3 follow-up, as above.

Changed to warn and added follow-up: #5696

JasonAtClockwork added 2 commits July 31, 2026 11:24

Audit of warn! and error!

205216b

Areas of concern for alerts

724a927

JasonAtClockwork commented Jul 31, 2026

View reviewed changes

Comment thread crates/client-api/src/routes/database.rs

JasonAtClockwork commented Jul 31, 2026

View reviewed changes

Comment thread crates/client-api/src/routes/subscribe.rs

JasonAtClockwork commented Jul 31, 2026

View reviewed changes

JasonAtClockwork marked this pull request as ready for review July 31, 2026 23:19

Merge branch 'master' into jlarabie/audit-logs

faad664

Signed-off-by: Jason Larabie <jason@clockworklabs.io>

JasonAtClockwork requested review from gefjon and joshua-spacetime August 4, 2026 14:58

gefjon mentioned this pull request Aug 4, 2026

ProcedureCallError: preserve distinction between guest and host errors #5660

Open

gefjon reviewed Aug 4, 2026

View reviewed changes

JasonAtClockwork mentioned this pull request Aug 6, 2026

Distinguish V1 WebSocket execution errors #5682

Open

JasonAtClockwork added 2 commits August 6, 2026 16:13

Fixing a couple notes

30c3990

Update comment for potential errors

22620e2

Cleanup from notes

550065b

		// TODO: Review log level after configured issuers can be distinguished from arbitrary token issuers.
		log::warn!("Error fetching public key for issuer {raw_issuer}: {e:?}");

		// TODO: Review log level after client SQL errors can be distinguished from internal database failures.
		log::error!("PG: Error for database {database}: {err}");

		// TODO: Do not log the supplied password/token; then classify credential errors separately from provider failures.
		log::error!(

Uh oh!

Conversation

JasonAtClockwork commented Jul 31, 2026

Description of Changes

API and ABI breaking changes

Expected complexity level and risk

Testing

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bfops commented Aug 3, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development